Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add camera GetProperties #18

Closed
wants to merge 1 commit into from
Closed

Conversation

edaniels
Copy link
Member

No description provided.

@edaniels edaniels requested a review from a team as a code owner April 11, 2024 14:05
@edaniels edaniels requested review from njooma and purplenicole730 and removed request for a team April 11, 2024 14:05
Copy link
Member

@purplenicole730 purplenicole730 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@njooma njooma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question

@@ -50,7 +54,15 @@ public static Camera fromRobot(final RobotClient robot, final String name) {
public abstract Image getImage(final Format format,
final Optional<Struct> extra);

public abstract Entry<List<Image>, Common.ResponseMetadata> getImages();
public Entry<List<Image>, Common.ResponseMetadata> getImages() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[q] why provide a default implementation here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was requested

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol k

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to point out that this could be a subtle point of bugs for anyone expecting getImages to work, but a camera implementer didn't provide it. This doesn't provide the user with any feedback (like an unimplemented error)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh yeah. ill change it back.

njooma
njooma previously approved these changes Apr 11, 2024
@njooma njooma self-requested a review April 12, 2024 18:06
@njooma
Copy link
Member

njooma commented May 2, 2024

@edaniels can you revert or can we close this?

@njooma njooma dismissed stale reviews from purplenicole730 and themself May 2, 2024 22:35

No longer approved

@edaniels edaniels closed this May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants